Skip to content

Conversation

crisbeto
Copy link
Member

Enables AoT compilation for all of our unit tests and fixes most of the errors that had accumulated over time.

@crisbeto crisbeto added the target: major This PR is targeted for the next major release label Sep 26, 2025
@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Sep 26, 2025
Enables AoT compilation for all of our unit tests and fixes most of the errors that had accumulated over time.
@crisbeto crisbeto marked this pull request as ready for review September 26, 2025 18:14
@crisbeto crisbeto requested a review from a team as a code owner September 26, 2025 18:14
@crisbeto crisbeto requested review from andrewseguin and tjshiu and removed request for a team September 26, 2025 18:14
}).toThrowError(/attempting to combine different/i);
});

it('should throw an error if a toolbar-row is added later', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test wasn't doing anything. Also the error is being thrown inside an observable so we can't really catch it synchronously here.

it('should throw when trying to assign an invalid position', () => {
expect(() => {
fixture.componentInstance.position = 'everywhere';
fixture.componentInstance.position = 'everywhere' as any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be TooltipPosition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is checking what happens if an invalid position is passed in.

@if (true) {
<mat-toolbar-row>First Row</mat-toolbar-row>
}
@if (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we have both of these in the conditional like before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because the control flow syntax affects content projection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why they can't be in the same if (true) block. Seems a little unintuitive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we had the new control flow, if you'd written something like <foo *ngIf="true"> it would be de-sugared to <ng-template [ngIf]="true"><foo/></ng-template>, however for content projection purposes Angular was copying the tag name and attributes of the foo tag to the ng-template tag. In order to avoid massive breaking changes, the new control flow does the same where the tag name/attributes will be copied onto the @if node. The only limitation is that this copying only happens if there's a single root node inside the @if. The compiler has a check that will flag cases where the @if will prevent content projection which was happening in this test. I split the element across two separate @if blocks so that they get content projected properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see - thanks for the detailed explanation.

template: `
<input [matDatepicker]="d">
<mat-datepicker-toggle tabIndex="7" [for]="d" [disabled]="disabled">
<mat-datepicker-toggle [tabIndex]="7" [for]="d" [disabled]="disabled">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a weird tabIndex number. Should this just be 1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we were testing it with a weird number to make sure takes effect, as opposed to 1 or 0 which could be a default.

@crisbeto crisbeto removed the request for review from andrewseguin September 26, 2025 20:24
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Sep 26, 2025
@crisbeto crisbeto merged commit a2383cf into angular:main Sep 26, 2025
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants